sql/event_log: reduce tech debt, simplify and fix bugs #64894
Conversation
|
This change is |
knz
force-pushed
the
20210507-event-log
branch
4 times, most recently
from
64a534e to
4b24c96
Compare
There was a problem hiding this comment.
so far i wasn't able to follow the code enough to identify where the fix to log the query string in exec_log happens. in general, the comments and refactors look good to me.
Reviewable status:
complete! 0 of 0 LGTMs obtained (waiting on @RichardJCai)
it was a mistake to do So instead the Does this explain? |
There was a problem hiding this comment.
I have rewritten the PR to decompose each logical change into a sub-commit, for your reviewing pleasure. I hope you like it?
Reviewable status:
There was a problem hiding this comment.
sorry i missed your updates while i was out of office at the end of last week. yes, this does help and it was pleasurable to review! lgtm
Reviewed 1 of 4 files at r1, 5 of 5 files at r8.
Reviewable status:
Prior to this patch, there was a little mess in `sql/event_log.go` that had been introduced when "optimizing" GRANT/REVOKE to only use one write batch for all the events logged: - the API interface for the functions in event_log.go were complex to use, requiring callers to provide descriptor IDs and structured events as separate slices; - the optimizing logic was not properly applied to the other case where multiple events are emitted: SQL audit logging in `exec_log.go`. This patch cleans this up by using the same struct `eventLogEntry` as argument to the various APIs. Release note: None
…ging The previous patch to batch event writes for GRANT/REVOKE had duplicated code. This was not necessary. This patch fixes this by using the same code for both cases. Release note: None
This clarifies its purpose. Release note: None
`logEventsOnlyExternally()` is specific to the code in `exec_log.go` and should thus reside there. This patch achieves that. Release note: None
The code was previously using two booleans `onlyLog` and `writeToEventLog` which were making the code difficult to understand and to maintain. This patch fixes this by introducing a bitset with descriptive names. Release note: None
This further simplifies the internal API. Release note: None
This patch describes `event_log.go` at a high level: an overall event refinement pipeline with a straightforward control flow. Release note: None
This patch fixes a bug introduced when query logging started using structured events: the ability to automatically copy all the execution events to the DEV channel when setting the `vmodule` setting to `exec_log=2` (or above). In addition to fixing that bug, the following new vmodule-based abilities are added: - events for DDL statements and others that call `logEvent()` can now be collected in the DEV channel by using the name of the source file where they were generated as filter (e.g. `vmodule=create_table=2` for the CREATE TABLE events. - events of other kinds can be collected in the DEV channel by setting `vmodule=event_log=2`. (Note a subtle difference between `vmodule=create_table=2` and `vmodule=exec_log=2`: the former emits the event to the DEV channel while the stmt is executed; the latter emits the event after the stmt completes. If both are enabled, TWO events are sent to the DEV channel.) Since all the vmodule filtering options are subject to change without notice between versions, we do not wish to document these nuances. For this reason, the release note below is left blank. Release note: None
|
TFYR! bors r=rafiss |
|
Build succeeded: |
Fixes #64685.
First commit from #64871.
Prior to this patch, there was a little mess in
sql/event_log.gothat had been introduced when "optimizing" GRANT/REVOKE to only
use one write batch for all the events logged:
were complex to use, requiring callers to provide descriptor
IDs and structured events as separate slices;
other case where multiple events are emitted:
SQL audit logging in
exec_log.go.This patch streamlines this by reducing
event_log.goback to itssimpler form: an overall event refinement pipeline with a
straightforward control flow.
To guide future maintainers, the patch also adds an explanatory
comment at the top of the file that sketches the overall structure of
the pipeline.
Finally, this patch fixes a bug introduced when query logging
started using structured events: the ability to automatically
copy all the execution events to the DEV channel when
setting the
vmodulesetting toexec_log=2(or above).In addition to fixing that bug, the following new vmodule-based
abilities are added:
logEvent()can nowbe collected in the DEV channel by using the name of the source file
where they were generated as filter (e.g.
vmodule=create_table=2forthe CREATE TABLE events.
by setting
vmodule=event_log=2.(Note a subtle difference between
vmodule=create_table=2andvmodule=exec_log=2: the former emits the event to the DEV channelwhile the stmt is executed; the latter emits the event after the stmt
completes. If both are enabled, TWO events are sent to the DEV channel.)
Since all the vmodule filtering options are subject to change without
notice between versions, we do not wish to document these nuances.
For this reason, the release note below is left blank.
Release note: None